Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Typed search attributes #1612

Merged
merged 24 commits into from
Mar 14, 2025
Merged

Typed search attributes #1612

merged 24 commits into from
Mar 14, 2025

Conversation

THardy98
Copy link
Contributor

@THardy98 THardy98 commented Jan 24, 2025 •
edited
Loading

What was changed

Introduces TypedSearchAttributes to the Typescript SDK, allowing users to supply and receive type-enforced search attributes. TypedSearchAttributes are intended to replace the existing (not type-enforced) SearchAttributes which have been marked as @deprecated in its existing usages.

Why?

Brings parity with other SDKs which have a typed search attribute implementation.
Brings parity to how the server represents search attributes.
Provides a typed, structured interface to use search attributes.

For reviewers

Points of interest:

  • typed-search-attributes.ts contains the API
  • test-typed-search-attributes.ts contains tests and demonstrates API usage
  • worflow.ts for changes to upsertSearchAttributes (mutations to search attributes in workflow info)
  • payload-search-attributes.ts contains decoding/encoding logic for search attributes (both legacy and typed)

  1. Closes [Feature Request] Typed Search Attributes #1232

  2. How was this tested:
    Added tests, see test-typed-search-attributes.ts

  3. Any docs updates needed?
    Likely as we are deprecating existing SearchAttributes

@THardy98 THardy98 requested a review from a team as a code owner January 24, 2025 13:39
@THardy98 THardy98 marked this pull request as draft January 24, 2025 13:39
@THardy98 THardy98 requested a review from mjameswh January 24, 2025 16:54
@THardy98
Copy link
Contributor Author

Most of the necessary revisions are done, fixing failing tests and adding new ones.

@THardy98 THardy98 marked this pull request as ready for review February 6, 2025 08:54
@THardy98
Type enforcement for typed search attributes
fee7333
@THardy98
improve doc strings
eee023e
@THardy98
formatting
f2399cc
@THardy98
fix build on merge
103b1b8
@THardy98
API change
25b0722 
- remove ITypedSearchAttributes, expose concrete class TypedSearchAttributes
- update constructor to take a list of pairs, consistent with the usage of pairs and class throughout the API
- `Options` objects (i.e. user input) allow for both pairs and the concrete class as input types
- created `TypedSearchAttributeUpdate`/`Pair` types, to be used for upserts and updates, the only difference with these types and the existing `TypedSearchAttribute/Pair` types is that they except `null` values (for deletion)
- fixed the `upsert` method to accomodate these types
- `updateSearchAttributes` now returns a new copy of `TypedSearchAttributes` to avoid mutation of existing `TypedSearchAttributes` objects
- a myriad of small changes to accomodate the type changes
@THardy98
added tests, fixed bugs caught by tests
78b74a4
@THardy98
linting and formatting
af0754e
@THardy98 THardy98 force-pushed the typed-search-attributes branch from e6c6b31 to af0754e Compare February 6, 2025 18:29
@THardy98
Copy link
Contributor Author

THardy98 commented Feb 6, 2025 •
edited
Loading

Updated the PR description with an overview for reviewers.

Some remaining points of uncertainty:

  • a couple TODO comments in places where I follow an existing pattern but am unsure if this is still the case (particularly those in payload-search-attributes.ts and internals.ts
  • I serialized expected responses in tests (i.e. JSON.parse(JSON.stringify(...))) to handle JSON converted responses from query. Figured this was okay, but just want to double-check
  • wondering if it's worth to test the isTypeSearchAttributePair type guard directly, or if the existing tests are sufficient

@THardy98 THardy98 force-pushed the typed-search-attributes branch from 9d727b3 to 3877147 Compare February 6, 2025 23:54
@THardy98 THardy98 force-pushed the typed-search-attributes branch from 3877147 to 3261083 Compare February 7, 2025 00:09
@THardy98 THardy98 requested a review from mjameswh February 7, 2025 00:32
@THardy98
Copy link
Contributor Author

THardy98 commented Feb 7, 2025

Who do I need to get in touch with to update docs? (considering we're deprecating a user-facing feature in exchange for another)

@THardy98
moving creation methods (creating keys, attribute pairs, etc.) to sta…
0c3afe2 
…tic methods within the class

renamed some types and methods
@THardy98 THardy98 force-pushed the typed-search-attributes branch from 1a601a1 to 0c3afe2 Compare February 7, 2025 22:58
@THardy98 THardy98 requested a review from mjameswh March 12, 2025 18:45
@mjameswh
Copy link
Contributor

We now have many linter warnings about usage of deprecated APIs. You will need to silence all of them by appending a // eslint-disable-line deprecation/deprecation comment at the end of those lines.

@THardy98
Copy link
Contributor Author

We now have many linter warnings about usage of deprecated APIs. You will need to silence all of them by appending a // eslint-disable-line deprecation/deprecation comment at the end of those lines.

There were a lot, I suppressed them inline, but is there a reason we don't do this in the eslint config instead?

@mjameswh
Copy link
Contributor

There were a lot, I suppressed them inline, but is there a reason we don't do this in the eslint config instead?

The "deprecation" rule plugin that we are using at the moment unfortunately doesn't have any level of configurability. And we really want to check for usage of deprecations, e.g. us using deprecated Node.js or third party libraries APIs.

mjameswh
mjameswh reviewed Mar 13, 2025
View reviewed changes
packages/test/package.json Outdated
@@ -8,7 +8,11 @@
"build:ts": "tsc --build",
"build:protos": "node ./scripts/compile-proto.js",
"test": "ava ./lib/test-*.js",
"test.watch": "ava --watch ./lib/test-*.js"
"test.watch": "ava --watch ./lib/test-*.js",
"test-split-one": "ava ./lib/test-integration-split-one.js",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly useful while developing locally, but I don't think we should commit such aliases. Too specic/volatile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, mistakenly commit - thanks

mjameswh
mjameswh approved these changes Mar 13, 2025
View reviewed changes
Copy link
Contributor

@mjameswh mjameswh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one last comment. Feel free to merge after that.

@THardy98 THardy98 force-pushed the typed-search-attributes branch from 496ffd9 to 5b75abd Compare March 13, 2025 23:25
@THardy98 THardy98 merged commit cca601e into main Mar 14, 2025
23 checks passed
@THardy98 THardy98 deleted the typed-search-attributes branch March 14, 2025 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjameswh mjameswh mjameswh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Typed Search Attributes
2 participants
@THardy98 @mjameswh